fix: don't hard-fail on an undecryptable saved password#10174
fix: don't hard-fail on an undecryptable saved password#10174kundansable wants to merge 2 commits into
Conversation
Connection._decode_password() let UnicodeDecodeError from decrypt() propagate as a hard error. For OIDC/OAuth2 users whose crypt key can differ between sessions, a password saved under a previous key can never be decoded, and every connection attempt surfaced "Failed to decrypt the saved password" — even after deleting and recreating the pgAdmin user, since the corrupted ciphertext is what's stored, not anything tied to the user record. Catch the decode failure, log a warning, and treat it as "no saved password" instead of propagating the error. connect() then falls through to the normal password prompt, so the account is recoverable instead of permanently stuck. Fixes pgadmin-org#10140
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe psycopg3 connection driver now treats saved-password decryption failures as non-fatal, clears cached password values, logs a warning, and allows the connection flow to request a password again. ChangesSaved password recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/pgadmin/utils/driver/psycopg3/connection.py (2)
261-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the recovery contract.
Test invalid ciphertext, valid encrypted passwords, passexec fallback, and an SSH-tunnel server with
save_password=True; assert that the invalid credential is not reused and the response prompts for a password.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 261 - 274, Extend the connection tests covering the saved-password handling around the decryption failure path to cover invalid ciphertext, valid encrypted passwords, passexec fallback, and an SSH-tunnel server configured with save_password=True. Assert that invalid credentials are discarded rather than reused and that the resulting response prompts the user for a password, while preserving successful reuse for valid encrypted passwords and existing passexec behavior.
255-274: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the exception scope.
except Exceptionconverts unrelated crypto-helper or programming failures into “bad saved password,” logs them as a warning, and continues withNone. Catch the concrete invalid-ciphertext/decryption exceptions plusUnicodeDecodeError, after verifying the repository’sdecrypt()contract, and let unexpected failures surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 255 - 274, In the password-decryption block of the connection logic, replace the broad `except Exception` with the concrete invalid-ciphertext/decryption exceptions raised by `decrypt()` plus `UnicodeDecodeError`, verifying the helper’s contract first. Preserve the existing warning and `(False, '', None)` fallback only for those expected bad-password cases, while allowing unrelated failures to propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 261-274: Update the invalid saved-password handling in the
connection credential flow to clear the cached ciphertext held by self.password
and return an explicit invalid-password state from the decryption helper. Ensure
connect() and the downstream get_response_for_password/SSH prompt flow consume
that state so passexec fallback is not skipped and the UI reliably prompts for a
new password.
---
Nitpick comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 261-274: Extend the connection tests covering the saved-password
handling around the decryption failure path to cover invalid ciphertext, valid
encrypted passwords, passexec fallback, and an SSH-tunnel server configured with
save_password=True. Assert that invalid credentials are discarded rather than
reused and that the resulting response prompts the user for a password, while
preserving successful reuse for valid encrypted passwords and existing passexec
behavior.
- Around line 255-274: In the password-decryption block of the connection logic,
replace the broad `except Exception` with the concrete
invalid-ciphertext/decryption exceptions raised by `decrypt()` plus
`UnicodeDecodeError`, verifying the helper’s contract first. Preserve the
existing warning and `(False, '', None)` fallback only for those expected
bad-password cases, while allowing unrelated failures to propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6638f40-221e-49e2-a5e5-2c2ff1ab7df6
📒 Files selected for processing (1)
web/pgadmin/utils/driver/psycopg3/connection.py
| # The saved password could not be decrypted. This happens | ||
| # when the stored ciphertext was encrypted with a different | ||
| # key (e.g. OIDC/OAuth2 logins where the derived encryption | ||
| # key changed between sessions), leaving un-decodable bytes | ||
| # (typically a "'utf-8' codec can't decode byte 0x.." error). | ||
| # Instead of failing every connection attempt permanently, | ||
| # discard the bad saved password and continue so the user is | ||
| # prompted for the password again. | ||
| current_app.logger.warning( | ||
| 'Ignoring the saved password as it could not be ' | ||
| 'decrypted. The user will be prompted for the password. ' | ||
| 'Error: {0}'.format(str(e)) | ||
| ) | ||
| return False, '', None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Propagate the invalid-saved-password state to the prompt flow.
Returning (False, '', None) at Line 274 does not clear the ciphertext or identify that the saved credential was unusable. connect() has already stored it in self.password, and the truthy encpass skips the passexec fallback at Lines 323-334. The supplied downstream handler also passes not server.save_password to get_response_for_password; for this stale record that is false, so the SSH path can return prompt_password=False. Clear the cached ciphertext and propagate an explicit invalid-password state so the UI reliably re-prompts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 261 - 274,
Update the invalid saved-password handling in the connection credential flow to
clear the cached ciphertext held by self.password and return an explicit
invalid-password state from the decryption helper. Ensure connect() and the
downstream get_response_for_password/SSH prompt flow consume that state so
passexec fallback is not skipped and the UI reliably prompts for a new password.
Per CodeRabbit review on PR pgadmin-org#10174: _decode_password() discarded the undecryptable password for the current decode call, but the stale ciphertext was already cached on self.password (set earlier in connect(), before decoding) and never cleared. That meant: - Every subsequent connect() attempt re-decoded and re-warned on the same bad ciphertext instead of the "no saved password" state sticking. - If a server also had passexec_cmd configured, the passexec fallback in connect() (which only runs when neither password nor encpass is set) was skipped, since the stale encpass was still truthy. Clear self.password and manager.password alongside discarding the password in _decode_password's except branch, so the bad ciphertext doesn't linger. The SSH-tunnel prompt path in get_response_for_password() trusting `not server.save_password` (which can be true even for an undecryptable saved password) is a separate, pre-existing issue and is intentionally left out of this narrower fix.
1 participant
Summary
Fixes #10140 — some OIDC/OAuth2 users intermittently get a hard
Failed to decrypt the saved passworderror on every connection attempt, and the account stays broken even after being deleted and recreated via OIDC.Root Cause
Connection._decode_password()callsdecrypt()(AES-CFB8, unauthenticated) on the stored password ciphertext and lets anyUnicodeDecodeErrorfrom a failed decrypt propagate as a hard error. For OIDC/OAuth2 logins, the crypt key used to encrypt a saved password can differ between sessions; if it does, the ciphertext saved under a previous key can never be decoded correctly under the current key, and every connection attempt hits this same decode failure — surfacing the scary, unrecoverable-looking "Failed to decrypt the saved password" error. Deleting and recreating the pgAdmin user doesn't help because the problem is in the stored ciphertext for the server record, not the user record.Fix
Catch the decode failure in
_decode_password(), log a warning, and return an empty/no-password result instead of propagating the exception — i.e. treat an undecryptable saved password the same as "no saved password was ever set."connect()then falls through to its normal password-prompt path, so the user is prompted for and can re-enter the correct password, making the account recoverable instead of permanently stuck. OIDC/OAuth2 crypt-key derivation itself is untouched by this change.Test Steps
(Exact repro requires an OIDC/OAuth2 setup where the crypt key can rotate between sessions; a close approximation can be done by directly corrupting a stored password's ciphertext in the config DB for a test server.)
passwordciphertext for that server record so it no longer decrypts under the current key.Summary by CodeRabbit